Updated return of matching TF and RID instructions #993
Conversation
Instead of returning a semicolon-delimited string, which must be parsed on the receiving end and cannot be easily converted to an item (with relevant metadata like which project it's associated with), return structured data in the only way MSBuild can: as an item with metadata. Metadata includes the closest matching TargetFramework and whether or not the project should have TargetFramework and/or RuntimeIdentifier stripped before calling it. Consumed by Microsoft.Common.CurrentVersion.targets in the coordinated change dotnet/msbuild#1866.
|
cc @nguerrera, @dotnet/msbuild |
| <ItemGroup> | ||
| <ProjectBuildInstructions Include="$(MSBuildProjectFullPath)"> | ||
| <DesiredTargetFrameworkProperties>TargetFramework=$(NearestTargetFramework)</DesiredTargetFrameworkProperties> | ||
| <HasSingleTargetFramework>$(_HasSingleTargetFramework)</HasSingleTargetFramework> |
There was a problem hiding this comment.
Since we're changing the protocol anyway, this name is misleading. It will be false for a project with <TargetFrameworks>xyz1.0</TargetFrameworks>, i.e. using TargetFrameworks plural but only specifying one value.
Should we generalize and just give the callee project the ability to specify properties to remove to avoid having to deal with these case-by-case?
There was a problem hiding this comment.
Yes, that's a really good idea.
There was a problem hiding this comment.
Ok, I really like this in theory but I'm worried it breaks when Project A cross-targets and depends on Project B (not an Sdk project). If Project B just has the common-targets placeholder for GetTargetFrameworkProperties, it won't return back that we should strip the TF and RID global variables--but we should. Right? Or is that a scenario we don't care about?
There was a problem hiding this comment.
Maybe I'm missing something, but that seems orthogonal. Today in this case we go to empty GetTargetFrameworkProperties, which won't set HasSingleTargetFramework or IsRidAgnostic.
Generally, it's not as bad to leave TargetFramework in place for non-SDK projects because they don't do anything with it. RuntimeIdentifier could be more problematic, but we have this problem today.
Instead of defaults for HasSingleTargetFramework and IsRidAgnostic, can we have a default set of properties to remove?
There was a problem hiding this comment.
Ok, that's possible, but I don't think I can get through implementing it today.
It's easy enough to make GetDesiredProperties return the right set of properties to set and to remove.
Where it's hard is removing the right properties for projects that want to short-circuit by saying "I don't cross-target, just strip the properties"--and not reevaluate with ReferringTF. We could change the protocol on ShouldQueryForProperties to instead get back an item with the project name and metadata for removed properties, basically the same way GetDesiredProperties does now. That's a harder thing to implement (especially in the default case) but not too bad.
@AndyGerlicher does that description make enough sense for you to pick this up?
There was a problem hiding this comment.
@AndyGerlicher I pushed https://github.com/rainersigwald/msbuild/tree/general-prop-set-and-unset-through-refs and https://github.com/rainersigwald/sdk/tree/general-prop-set-and-unset-through-refs with a WIP of going down this road. It almost works--but only for cross-targeting projects. It doesn't strip TF/RID from refs to single-targeting projects.
What should be done next to go down this road is to change ShouldQueryForProperties to return the same type of thing that GetDesiredProperties, except no SetDesiredProperties--we only care about UndefineProperties, which should be set for all future calls to that ProjectReferenceExistent.
| <PropertyGroup> | ||
| <!-- indicate to caller that project is RID agnostic so that a global property RuntimeIdentifier value can be removed --> | ||
| <_IsRidAgnostic>false</_IsRidAgnostic> | ||
| <_IsRidAgnostic Condition=" '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' ">true</_IsRidAgnostic> |
There was a problem hiding this comment.
I believe this will not deal well with the case where RuntimeIdentifier is specified as a global property (-r from CLI). We removed it from global properties for GetTargetFrameworkProperties, but not from ShouldQueryForProperties :(
EDIT: Nevermind, they are removed from this evaluation as well.
This is a compat shim, and should be reverted once dotnet/msbuild#1866 is available for use in the SDK. It will not be used with common targets that contain that change.
|
Latest commit makes this compatible with old common targets, so it's not a breaking change. This can be merged, then an updated MSBuild adopted, then b0cf048 reverted. |
|
|
||
| <ItemGroup> | ||
| <ProjectBuildInstructions Include="$(MSBuildProjectFullPath)"> | ||
| <DesiredTargetFrameworkProperties>TargetFramework=$(NearestTargetFramework)</DesiredTargetFrameworkProperties> |
There was a problem hiding this comment.
If we're renaming the target, shouldn't we also remove TargetFramework from this part of the contract?
|
Latest iteration (here and in dotnet/msbuild#1866) changes the protocol again so that a referenced project can unset properties in the initial @nguerrera @livarcocc I'd love to test the coordinated change between the MSBuild repo and the SDK repo, but I can't figure out how to get the private common targets into the sdk repo for testing. I'm stuck at the moment because the common targets come in through the CLI and not a direct ref to MSBuild so I can't just update that. Can one of y'all help me work that out? |
|
Thanks to @livarcocc for helping me test the new targets in the SDK repo (by manually patching the downloaded CLI with the new targets after download, so it gets used for all the tests). That revealed that this does have one major change from the current always-query approach: there's no longer an easy way to determine that the remote project's single TF is not compatible with a referencing project. I filed #1122 so we can talk about that without the legacy of design iterations in this PR. |
|
MSBuild triage: not pursuing this for now due to the TF-compat check and other complexity concerns. |
…0191008.20 (dotnet#993) - Microsoft.AspNetCore.Analyzers - 3.1.0-preview1.19508.20 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.1.0-preview1.19508.20 - Microsoft.AspNetCore.Mvc.Analyzers - 3.1.0-preview1.19508.20 - Microsoft.AspNetCore.Components.Analyzers - 3.1.0-preview1.19508.20
Instead of returning a semicolon-delimited string, which must be parsed
on the receiving end and cannot be easily converted to an item (with
relevant metadata like which project it's associated with), return
structured data in the only way MSBuild can: as an item with metadata.
Metadata includes the closest matching TargetFramework and whether or
not the project should have TargetFramework and/or RuntimeIdentifier
stripped before calling it.
Consumed by Microsoft.Common.CurrentVersion.targets in the coordinated
change dotnet/msbuild#1866.